FIX: SLEEF migration to 4.0 #240
Conversation
|
Took the opportunity to also clean the building process and warnings |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
It's unfortunate the parsing code had to change so much. It might be a little clearer if you do a first PR that does some refactoring in the parsing code, then rebased this PR on top of that one. Up to you if you feel like it's worth it.
I see you replaced cstring_to_quad with NumPyOS_ascii_strtoq - do we ever want to user the former anymore? If so, when? Maybe worth adding as a comment somewhere.
|
|
||
| // Call Sleef_strtoq with the bounded string | ||
| char *sleef_endptr; | ||
| out_value->sleef_value = Sleef_strtoq(temp, &sleef_endptr); |
There was a problem hiding this comment.
this is pretty subuptimal :(
Consider refactoring all the code you added here into a function even if there's only one caller, since it's pretty distracting here inline in this function.
|
|
||
| @pytest.mark.parametrize("invalid_bytes", [ | ||
| b"1.0 ", # Trailing whitespace | ||
| b"1.0 ", # Multiple trailing spaces |
There was a problem hiding this comment.
why delete test cases? it seems like the parsing code changed a lot above, if anything I'd expect added test cases here.
There was a problem hiding this comment.
Because even if for scalars, numpy and python handle the trailing whitespace correctly and parse the input
and here these are the tests that tests the invalid inputs (meant to be failed)
Reported it here shibatch/sleef#706 |
It seems on conda nightly for numpy-2.5 does not have py-11 wheels. Since it's just for typecheck workflow I think for simple use I can just bump the python requirement there instead of setting to build from repo |
|
So tl;dr I refactored the string parsing logic and now every call site is using SLEEF 4.0's cc: @ngoldbaum I hope you like this setup |
| def __add__(self, other: _IntoQuad, /) -> Self: ... # type: ignore[override] # pyright: ignore[reportIncompatibleMethodOverride] | ||
| @override | ||
| def __radd__(self, other: _IntoQuad, /) -> Self: ... # type: ignore[override] # pyright: ignore[reportIncompatibleMethodOverride] | ||
| def __radd__(self, other: _IntoQuad, /) -> Self: ... # type: ignore[override, misc] # pyright: ignore[reportIncompatibleMethodOverride] |
There was a problem hiding this comment.
Nothing wrong with this, so I agree that mypy should shut up here
4 participants
closes #239
Saturday spend well! Some points to mention:
utilities.cand their corresponding calls have been modified because of the modified behaviour ofSleef_strtoqand handling of negative NaN.tlfloat43a0252), not setting tomasteras maybe in future more routine behaviour changes can break the dtype here (Ideally in future it'll be better to migrate the core changes from master if something fundamental breaks)QBLASstill uses them, I'll update them there and then we can maybe update here or keep it as it is